SNI extension by ashfall · Pull Request #105 · python-tls/tls

ashfall · 2016-11-17T06:46:02Z

Implementing as specified in https://tools.ietf.org/html/rfc6066#section-3

ashfall · 2016-11-17T06:47:22Z

tls/_constructs.py

+        {
+            enums.NameType.HOST_NAME: HostName
+        }
+    )


Perhaps EnumSwitch would go better here

Agreed!

EDIT I just realized that comment wasn't clear -- sorry! I meant to say that it's OK to make this use EnumSwitch in this PR, but I'm happy to see this addressed in a separate PR that closes #106.

coveralls · 2016-11-17T06:48:53Z

Coverage remained the same at 100.0% when pulling 412e913 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-17T06:48:53Z

Coverage remained the same at 100.0% when pulling 412e913 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-17T06:59:05Z

Coverage remained the same at 100.0% when pulling 406f198 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-17T07:03:33Z

Coverage remained the same at 100.0% when pulling f3663a6 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-17T07:03:33Z

Coverage remained the same at 100.0% when pulling f3663a6 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-17T07:05:07Z

Coverage remained the same at 100.0% when pulling f3663a6 on ashfall:sni-extension into 67c2ef3 on pyca:master.

ashfall · 2016-11-17T07:55:45Z

I'm not very happy about doing server_name_list[0].name.data and would rather 'localhost' be server_name_list[0].name, i.e. not have the HostName container as a thing. From the RFC (https://tools.ietf.org/html/rfc6066):

      struct {
          NameType name_type;
          select (name_type) {
              case host_name: HostName;
          } name;
      } ServerName;

      enum {
          host_name(0), (255)
      } NameType;

      opaque HostName<1..2^16-1>;

      struct {
          ServerName server_name_list<1..2^16-1>
      } ServerNameList;

Thoughts?

coveralls · 2016-11-17T08:00:28Z

Coverage remained the same at 100.0% when pulling df5c174 on ashfall:sni-extension into 67c2ef3 on pyca:master.

markrwilliams

Looks good! I addressed your comment in my review.

Otherwise this looks great! I'm looking forward to the next review.

Thanks so much for getting extension parsing working!!

markrwilliams · 2016-11-17T14:47:13Z

tls/_constructs.py

+        {
+            enums.NameType.HOST_NAME: HostName
+        }
+    )


Agreed!

EDIT I just realized that comment wasn't clear -- sorry! I meant to say that it's OK to make this use EnumSwitch in this PR, but I'm happy to see this addressed in a separate PR that closes #106.

markrwilliams · 2016-11-17T14:50:20Z

tls/_constructs.py

    Array(lambda ctx: ctx.length, UBInt8("compression_methods"))
 )

+HostName = Struct(


From @ashfall's comment on this PR:

I'm not very happy about doing server_name_list[0].name.data and would rather 'localhost' be server_name_list[0].name, i.e. not have the HostName container as a thing.

I think you can remove the Struct and name the PrefixedBytes hostname:

HostName = PrefixedBytes("hostname", UBInt16("length"))

But I haven't tested it.

coveralls · 2016-11-17T19:11:46Z

Coverage decreased (-0.08%) to 99.924% when pulling 085f5d6 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-17T19:11:46Z

Coverage decreased (-0.08%) to 99.924% when pulling 085f5d6 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-17T19:11:46Z

Coverage decreased (-0.08%) to 99.924% when pulling 085f5d6 on ashfall:sni-extension into 67c2ef3 on pyca:master.

markrwilliams

A question and a docstring spelling issue.

Thanks so much for working on this!

markrwilliams · 2016-11-19T20:40:50Z

tls/test/test_hello_message.py

-        :func:`parse_server_hello` returns an instance of :class:`ServerHello`
-        with extensions, when the extension bytes are present in the input.
+        :func:`parse_server_hello` fails to parse when
+        SIGNATURE_ALGORITHMS extention bytes are present in the packet


Looks like the docs build failure is due to extention:

https://travis-ci.org/pyca/tls/jobs/176794154#L433

markrwilliams · 2016-11-19T20:43:41Z

tls/hello_message.py

        :return: ClientHello object.
        """
        construct = _constructs.ClientHello.parse(bytes)
+        if any(extension.type not in cls.allowed_extensions


This was an excellent catch! Should as_bytes also have this check, so that it's not possible to serialize a hello message that contains invalid extensions?

ashfall · 2016-11-20T14:00:00Z

@markrwilliams Thanks for the review, updates coming!

I have a question for you about the EnumSwitch API. In order to satisfy the coverage requirement, I was writing a test with an unsupported extension in ClientHello:

    def test_client_hello_fails_with_unsupported_extension(self):
        """
        :py:func:`tls.hello_message.ClientHello` does not parse a packet
        with an unsupported extension, and raises an error.
        """
        server_certificate_type_extension_data = (
            b'\x00\x14'  # Extension Type: Server Certificate Type
            b'\x00\x00'  # Length
            b''  # Data
        )

        client_hello_packet = self.common_client_hello_data + (
            b'\x00\x04'  # extensions length
        ) + server_certificate_type_extension_data

        with pytest.raises(UnsupportedExtensionException):
            ClientHello.from_bytes(
                client_hello_packet
            )

Since we haven't implemented the server_certificate_type extension yet, Extension raised a SwitchError: no default case defined on parsing server_certificate_type_extension_data.

To define a default, EnumSwitch says:

    :param value_choices: A dictionary that maps members of
        `type_enum` to subconstructs.  This follows
        :py:func:`construct.core.Switch`'s API, so ``_default_`` will
        match any members without an explicit mapping.
    :type value_choices: :py:class:`dict`

and it was unclear to me how/where to provide the __default__.

To make my test pass, I hacked around a bit and made the following changes:

I modified EnumSwitch slightly, a rough implementation is below (the if-else is ugly, I should clean that up):

def EnumSwitch(type_field, type_enum, value_field, value_choices, default=None): 
    if not default:  # we don't want to overwrite Switch's default NoDefault (https://github.com/construct/construct/blob/master/construct/core.py#L1550)
        return (EnumClass(type_field, type_enum),
                construct.Switch(value_field,
                                 operator.attrgetter(type_field.name),
                                 value_choices))
    else:  
        return (EnumClass(type_field, type_enum),
                construct.Switch(value_field,
                                 operator.attrgetter(type_field.name),
                                 value_choices,
                                 default=default))

And added a default construct.Pass to the Extension construct:

Extension = Struct(
    "extensions",
    *EnumSwitch(
        type_field=UBInt16("type"),
        type_enum=enums.ExtensionType,
        value_field="data",
        value_choices={
            enums.ExtensionType.SERVER_NAME: Opaque(ServerNameList),
            enums.ExtensionType.SIGNATURE_ALGORITHMS: Opaque(
                SupportedSignatureAlgorithms
            ),
        },
        default=Pass,
    )
)

This made test_client_hello_fails_with_unsupported_extension pass.

All the hackery aside, is there an existing way to provide default values to EnumSwitch?

coveralls · 2016-11-20T14:42:02Z

Coverage decreased (-0.08%) to 99.924% when pulling 9d1e243 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-20T14:42:02Z

Coverage decreased (-0.08%) to 99.924% when pulling 9d1e243 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-20T14:42:02Z

Coverage decreased (-0.08%) to 99.924% when pulling 9d1e243 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-20T14:42:02Z

Coverage decreased (-0.08%) to 99.924% when pulling 9d1e243 on ashfall:sni-extension into 67c2ef3 on pyca:master.

coveralls · 2016-11-20T19:21:00Z

Coverage decreased (-0.08%) to 99.925% when pulling 1edcbed on ashfall:sni-extension into 67c2ef3 on pyca:master.

…er hello. Also fix the bug where signature_algorithm extension was parsed and tested with serverhello.

coveralls · 2016-11-23T06:31:36Z

Coverage remained the same at 100.0% when pulling 7c77cc8 on ashfall:sni-extension into 5d48cc4 on pyca:master.

markrwilliams · 2016-11-26T18:59:28Z

@ashfall This looks great! I'm going to merge. We can address any lingering concerns in subsequent PRs.

Thanks so much for implementing this!

ashfall commented Nov 17, 2016

View reviewed changes

markrwilliams reviewed Nov 17, 2016

View reviewed changes

markrwilliams reviewed Nov 19, 2016

View reviewed changes

ashfall mentioned this pull request Nov 21, 2016

Allow the EnumSwitch API to take a default #107

Merged

ashfall added 9 commits November 23, 2016 11:57

Define ServerNameList extension constructs

48ec7fb

Implmenet constructs for server_name extension

53af807

a B for py3

83c26cc

Remove unused import

07534f8

Extra whitespace

4e4541c

pep8

3562b89

HostName doesn't need to be a struct

7f0488c

Add a failing test for server_name extension's presence in ServerHello

f836618

Correctly validate extensions that should go in client hello vs. serv…

4538855

…er hello. Also fix the bug where signature_algorithm extension was parsed and tested with serverhello.

ashfall added 4 commits November 23, 2016 11:58

Sometimes, I can spell.

01b6f2a

Add a check for unsupported extensions in ServerHello.as_bytes()

8ac0124

Test ClientHello.from_bytes() with an unsupported extension

5de80f8

Reject unsupported extensions in ClientHello.as_bytes()

7c77cc8

ashfall force-pushed the sni-extension branch from f80712a to 7c77cc8 Compare November 23, 2016 06:28

markrwilliams merged commit 5a7a0ec into python-tls:master Nov 26, 2016

ashfall deleted the sni-extension branch December 9, 2016 07:05

markrwilliams mentioned this pull request Dec 11, 2016

Link to relevant RFC sections in tls._constructs #122

Open

Conversation

ashfall commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashfall Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

markrwilliams Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 17, 2016

Uh oh!

coveralls commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 17, 2016

Uh oh!

coveralls commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashfall commented Nov 17, 2016

Uh oh!

coveralls commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markrwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

markrwilliams Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markrwilliams Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 17, 2016

Uh oh!

coveralls commented Nov 17, 2016

Uh oh!

markrwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

markrwilliams Nov 19, 2016

Choose a reason for hiding this comment

Uh oh!

markrwilliams Nov 19, 2016

Choose a reason for hiding this comment

Uh oh!

ashfall commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 20, 2016

Uh oh!

coveralls commented Nov 20, 2016

Uh oh!

coveralls commented Nov 20, 2016

Uh oh!

coveralls commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markrwilliams commented Nov 26, 2016

Uh oh!

Reviewers

Assignees

ashfall commented Nov 17, 2016 •

edited

Loading

markrwilliams Nov 17, 2016 •

edited

Loading

coveralls commented Nov 17, 2016 •

edited

Loading

coveralls commented Nov 17, 2016 •

edited

Loading

coveralls commented Nov 17, 2016 •

edited

Loading

coveralls commented Nov 17, 2016 •

edited

Loading

coveralls commented Nov 17, 2016 •

edited

Loading

markrwilliams Nov 17, 2016 •

edited

Loading

coveralls commented Nov 17, 2016 •

edited

Loading

ashfall commented Nov 20, 2016 •

edited

Loading

coveralls commented Nov 20, 2016 •

edited

Loading

coveralls commented Nov 20, 2016 •

edited

Loading

coveralls commented Nov 23, 2016 •

edited

Loading